Code coverage #25

sfdctaka · 2023-06-18T00:17:35Z

Code coverage can now be output with: npm run test-coverage

Using nyc to instrument the code programatically. nyc is CLI of istanbul. Running nyc from the command line, like nyc npm run test, for vscode extension project in Typescript doesn't pickup the behavior defined in nyc's configuration file. So instead nyc had to be used programatically. This alleviates from doing a full implementation using istanbul APIs to generate code coverage output and then use that output to generate the report.

With the code coverage enablement every PR will have % of code covered. This is made possible by an off the shelf GitHub action called Code Coverage Summary. This GitHub action doesn't generate badge for the readme though. I looked around to do that but I think it is probably good for the team to first discuss on that.

Another interesting thing in this PR is the need to use another GitHub action in Setup Headless Display Action. This was needed because VSCode extension test, which calls runTestmethod to initiate the test, will download VSCode, installs that, and then spawns a new instance of VSCode. This fails on CI machine without a monitor attached. Setup Headless Display Action fixes this.

salesforce-cla · 2023-06-18T00:17:39Z

Thanks for the contribution! Before we can merge this, we need @invalid-email-address to sign the Salesforce Inc. Contributor License Agreement.

sfdctaka · 2023-06-18T01:52:47Z

src/test/suite/commands/configureProjectCommand.test.ts

+        await CommonUtils.executeCommandAsync(
+            'git config --global user.email "[email protected]"'
+        );
+        await CommonUtils.executeCommandAsync(
+            'git config --global user.name "Your Name"'
+        );


On CI user name and email need to be set. Otherwise it was failing.

Okay, I need to fix this because running these lines changed my git user name and email on my machine.

Fixed it. Checking existence of user name and email first. If they don't error is thrown and try/catch is used to set temporary values.

sfdctaka · 2023-06-18T01:55:03Z

src/test/suite/index.ts

+// @ts-ignore
+import * as baseConfig from '@istanbuljs/nyc-config-typescript';


Without @ts-ignore I was getting lint error saying I need to add d.ts for this module. So suppressing it with @ts-ignore.

dbreese

I have an odd result when I run this from your branch. No matter what Ive tried, it is still picking up two old, non-existent directories, src/landingPage, and src/messages, despite git status showing no differences. I removed the out and node_modules dir and rebuilt them, too and recompiled. Really odd and I dont know where this is being picked up from.

dbreese · 2023-06-19T12:51:04Z

I did a fresh checkout and the issue goes away. I wonder where that state is being stored of the old files?

github-actions · 2023-06-19T12:52:22Z

Package	Line Rate	Branch Rate	Health
src	50%	38%	❌
src.commands	73%	56%	➖
src.utils	91%	61%	✔
Summary	74% (262 / 355)	55% (50 / 91)	➖

Minimum allowed line rate is 60%

github-actions · 2023-06-19T12:52:23Z

Package	Line Rate	Branch Rate	Health
src	50%	38%	❌
src.commands	73%	56%	➖
src.utils	91%	61%	✔
Summary	74% (262 / 355)	55% (50 / 91)	➖

Minimum allowed line rate is 60%

dbreese

LGTM! If we delete the landingPageCommand.ts which is not being used, coverage will go up. We can always grab it from history when/if we decide to use it.

khawkins · 2023-06-19T16:25:34Z

src/test/suite/index.ts

+// @ts-ignore
+import * as baseConfig from '@istanbuljs/nyc-config-typescript';
+
+const NYC = require('nyc');


Any reason not to make this an ES6-style import here?

I actually tried import but Typescript compiler would complain I need d.ts. Then I tried looking for @types/nyc but it didn't exist so I decided to go with require.

khawkins · 2023-06-19T16:29:19Z

.github/workflows/run-tests.yml

@@ -13,8 +13,33 @@ jobs:
      - uses: actions/setup-node@v3
        with:
          node-version: ${{ matrix.node }}
+      - uses: pyvista/setup-headless-display-action@v1


This (and CodeCoverageSummary, sticky-pull-request-comment, and setup-headless-display-action) are likely things we'll need to file 3PPs for, and make our ProdSec reviewers aware of, etc.

Couple things that come to my mind. One is time constraint. Do we want to go through the process of 3PP when we have a milestone to make.

The second is code coverage itself. Do we need it? Or is it a nice-to-have at this point? If we don't need coverage we can only do 3PP for setup-headless-display-action now.

I'm most especially wary of consuming third-party software in GitHub Actions, as there's considerable risk in doing so—those actions by default have the same permissions on your repo as the user who installed them.

I don't feel we should use them without a) good reason, b) proper vetting, and c) robust guardrails to curtail the possibility of security risk.

Let me check again what salesforcedx does again.

The reason we need it is because runTests will download/install/launch an instance of VSCode. If we can move our code to be testable with jest we can avoid using runTests.

With regards to launching VSCode, this is something we could do directly. If you look at an example for Azure Pipelines, they stand up an Xvfb server that can host the VSCode process. This is a process we can do in a GitHub Action as well. It's largely what a lot of these 3PP actions are doing behind the scenes.

I'd rather we invest in first-party code specifically for GitHub Actions, because of the risks. To me, if that means things like test runners and code coverage reporting needs to push past V1, in favor of running locally, I'd personally prefer that to rushing into deploying these only to end up with security risks as a result.

I'm game for leveraging Actions code that comes directly from GitHub/Microsoft (i.e. under https://github.com/actions), because at that point you're trusting the provider that's hosting all of your code and secrets anyway.

Let me try the Xvfb server sample on Azuer Pipeline now. If this works we can remove setup-headless-display-action .

sfdctaka added 8 commits June 16, 2023 20:47

Use NYC to instrument programatically.

a0a420c

Added npm packages. Prettier. nit.

13d82de

First cleanup.

b1f0feb

Cleanup.

e0a0649

Prettier.

9c89283

Split test and test coverage. Turn back on strict.

cb44a2a

Add test step.

5f1c108

Headless CI.

97a4ad8

salesforce-cla bot added the cla:missing label Jun 18, 2023

sfdctaka force-pushed the codeCoverage branch 2 times, most recently from 01d458c to 6905ee5 Compare June 18, 2023 00:30

sfdctaka changed the title ~~[DO NOT REVIEW YET] Code coverage~~ Code coverage Jun 18, 2023

sfdctaka marked this pull request as ready for review June 18, 2023 01:48

sfdctaka requested a review from a team as a code owner June 18, 2023 01:48

sfdctaka commented Jun 18, 2023

View reviewed changes

Code coverage summary.

bd33aed

sfdctaka force-pushed the codeCoverage branch from 42d4dbd to bd33aed Compare June 18, 2023 04:04

salesforce-cla bot removed the cla:missing label Jun 18, 2023

nit.

44f85ec

dbreese reviewed Jun 19, 2023

View reviewed changes

dbreese closed this Jun 19, 2023

dbreese reopened this Jun 19, 2023

dbreese approved these changes Jun 19, 2023

View reviewed changes

khawkins reviewed Jun 19, 2023

View reviewed changes

sfdctaka force-pushed the codeCoverage branch 25 times, most recently from 904b8be to c30e833 Compare June 20, 2023 22:44

Remove code coverage github actions. Use xvfb for display.

b280bfa

sfdctaka force-pushed the codeCoverage branch from c30e833 to b280bfa Compare June 20, 2023 22:48

khawkins approved these changes Jun 20, 2023

View reviewed changes

sfdctaka merged commit e2408ce into main Jun 21, 2023

sfdctaka deleted the codeCoverage branch June 21, 2023 00:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Code coverage #25

Code coverage #25

sfdctaka commented Jun 18, 2023 •

edited

Loading

salesforce-cla bot commented Jun 18, 2023

sfdctaka Jun 18, 2023

sfdctaka Jun 18, 2023

sfdctaka Jun 18, 2023

sfdctaka Jun 18, 2023

dbreese left a comment

dbreese commented Jun 19, 2023

github-actions bot commented Jun 19, 2023

github-actions bot commented Jun 19, 2023

dbreese left a comment

khawkins Jun 19, 2023

sfdctaka Jun 19, 2023

khawkins Jun 19, 2023

sfdctaka Jun 19, 2023

khawkins Jun 19, 2023

sfdctaka Jun 19, 2023

khawkins Jun 19, 2023

khawkins Jun 19, 2023

sfdctaka Jun 19, 2023

		// @ts-ignore
		import * as baseConfig from '@istanbuljs/nyc-config-typescript';

Code coverage #25

Code coverage #25

Conversation

sfdctaka commented Jun 18, 2023 • edited Loading

salesforce-cla bot commented Jun 18, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbreese left a comment

Choose a reason for hiding this comment

dbreese commented Jun 19, 2023

github-actions bot commented Jun 19, 2023

github-actions bot commented Jun 19, 2023

dbreese left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfdctaka commented Jun 18, 2023 •

edited

Loading